Skip to content

CURATOR-699. Upgrade ZooKeeper version to 3.9 - #496

Merged
tisonkun merged 6 commits into
apache:masterfrom
tisonkun:zk39
Feb 15, 2024
Merged

CURATOR-699. Upgrade ZooKeeper version to 3.9#496
tisonkun merged 6 commits into
apache:masterfrom
tisonkun:zk39

Conversation

@tisonkun

@tisonkun tisonkun commented Jan 31, 2024

Copy link
Copy Markdown
Member

No description provided.

@tisonkun
CURATOR-XXX. Upgrade ZooKeeper version to 3.9
40c13ed 
Signed-off-by: tison <wander4096@gmail.com>
tisonkun
tisonkun commented Jan 31, 2024
View reviewed changes
Comment thread ...ecipes/src/test/java/org/apache/curator/framework/recipes/leader/ChaosMonkeyCnxnFactory.java
ByteBuffer duplicate = si.request.duplicate();
duplicate.rewind();
ByteBufferInputStream.byteBuffer2Record(duplicate, createRequest);
CreateRequest createRequest = si.readRequestRecord(CreateRequest::new);

@tisonkun tisonkun Jan 31, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause failures on early ZK versions and we need a testing compatible layer.

Let the CI verify if it's the case.

@tisonkun tisonkun Feb 1, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. It fails.

Let me think if we should drop support <= 3.7 or add a test compatible switch.

cc @kezhuw @eolivelli @cammckenzie @Randgalt

@eolivelli

eolivelli commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

I think that it is fine to drop support for 3.7 in those tests

@tisonkun

tisonkun commented Feb 1, 2024

Copy link
Copy Markdown
Member Author

Make sense. I understand it as introducing a new test tag said zk38OrLater to turn on related tests only when zk 3.8+ as dep.

@tisonkun
try add zk38OrLater tag
0e75fa8 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
try refactor
ed12072 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun

tisonkun commented Feb 14, 2024

Copy link
Copy Markdown
Member Author 
[ERROR] Tests run: 3, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 0.44 s <<< FAILURE! - in org.apache.curator.framework.imps.TestTransactionsOld
[ERROR] org.apache.curator.framework.imps.TestTransactionsOld.testWithCompression  Time elapsed: 0.144 s  <<< ERROR!
java.lang.NoSuchMethodError: 'com.google.common.base.Predicate org.apache.curator.framework.api.transaction.CuratorTransactionResult.ofTypeAndPath(org.apache.curator.framework.api.transaction.OperationType, java.lang.String)'
	at org.apache.curator.framework.imps.TestTransactionsOld.testWithCompression(TestTransactionsOld.java:165)

It seems because we relocate these methods in curator-framework but not for the test-jar. I don't find a way to relocate the test jar now ..

@tisonkun

tisonkun commented Feb 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

I create a monkey patch to work around it - c44e798

@tisonkun
monkey patch
c44e798 
Signed-off-by: tison <wander4096@gmail.com>
eolivelli
eolivelli approved these changes Feb 14, 2024
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@tisonkun
correct groups
d101aee 
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun

tisonkun commented Feb 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@eolivelli Thanks for your review. I make this patch work now. Let me create the ticket and a few follow-ups to improve the case (It now takes 2 hours to complete, I believe we can drop a few versions, but better with a few separate tickets).

@tisonkun tisonkun changed the title CURATOR-XXX. Upgrade ZooKeeper version to 3.9 CURATOR-699. Upgrade ZooKeeper version to 3.9 Feb 15, 2024
@tisonkun
tisonkun merged commit 972fffa into apache:master Feb 15, 2024
@tisonkun
tisonkun deleted the zk39 branch February 15, 2024 02:30
@tisonkun tisonkun mentioned this pull request Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tisonkun @eolivelli